Conversation
This comment has been minimized.
This comment has been minimized.
e4c08f7 to
3b1957a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in cc @BoxyUwU Some changes occurred in match checking cc @Nadrieril This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @vakaras This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to constck cc @fee1-dead Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the CTFE machinery Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
r? compiler |
| } | ||
|
|
||
| adjustment::Adjust::GenericReborrow(_reborrow) => { | ||
| // Do the magic! |
There was a problem hiding this comment.
Oh we should call delegates. See the ReborrowPin arm and that is basically what we want to do.
And eventually we should be able to subsume ReborrowPin case hopefully.
| // `&mut T: DerefMut` tho, so it's kinda moot. | ||
| } | ||
| Adjust::GenericReborrow(_) => { | ||
| // No effects to enforce here. |
There was a problem hiding this comment.
We should do something here. The type would change after a generic reborrow, ie the CoerceShared case.
|
Where can I read context on what CoerceShared is/why it exists, and what the design for reborrow traits is |
I added a link to the experiment tracking issue but put it very shortly: |
|
I think I'll just |
Thank you <3 my top secret plan (designed together with tmandry) was to get a compiler review and then reroll into types to that view as well so that works well :) |
Appreciated. |
|
given oli has more context on this, (though if you can't review this we should chat about it and figure out what to do here :>) |
|
|
| WrapUnsafeBinder(Operand<'tcx>, Ty<'tcx>), | ||
|
|
||
| /// Creates a bitwise copy of the indicated place with the same type (if Mut) or its | ||
| /// CoerceShared target type (if Not), and disables the place for writes (and reads, if Mut) for |
There was a problem hiding this comment.
What does "disables the place" mean?
The first half sounds like a description of the operational semantics, but the second half sounds like a description of what happens in the borrow checker. It's quite confusing to mix those together.
There was a problem hiding this comment.
Same thing as what b does for a here:
let a = &mut 0u32;
let b = &mut *a;a is "disabled" for reads and writes while b lives. So this is borrow checker stuff indeed; I assumed that'd be relevant information here. And I must admit I assumed it is fundamentally an opsem consideration; is &mut XOR & not part of opsem?
If I split this up, what sort of split would you expect? Would the first half explain simply that for a btiwse copy is produced (and that in the future Not reborrows may include reordering and dropping of fields), and then the second half explain that the borrow checker creates a covariant lifetime relation between the source and target's only lifetime (and that in the future multiple lifetimes may participate in the reborrowing, in which case all source lifetimes are covariantly related to their corresponding pair on the target)?
There was a problem hiding this comment.
It is relevant but it is not opsem. You implemented the opsem in this PR when you added support in interpret and you didn't do anything wrt disabling the source there did you? :)
You can just have two paragraphs, one for what it means for the borrow checker and one for what it means operationally.
There was a problem hiding this comment.
(this is still unresolved, so the PR is not fully ready yet)
There was a problem hiding this comment.
Sorry, mildly forgot. Fixed now.
Let me know if this is not still up to snuff in your view! <3
There was a problem hiding this comment.
Stuck somewhere in the middle of a review, but submitting at this point as I gotta run
I have come back to "this should just be a BorrowKind::Custom", which I don't think will affect perf or complexity (and def not more than this PR does), but I can finish reviewing if you want before doing such a major refactor of the PR
| self.sub_types(rv_ty, place_ty, location.to_locations(), category) | ||
| // Note: we've checked Reborrow/CoerceShared type matches | ||
| // separately in fn visit_assign. | ||
| if !matches!(rv, Rvalue::Reborrow(_, _)) |
There was a problem hiding this comment.
I think we should not have a visit_assign in this visitor, but instead do an if/else chain here to handle the generic reborrow case
There was a problem hiding this comment.
Didn't do this just yet.
There was a problem hiding this comment.
Actually, I'm not sure if we can do this: visit_assign is called in eg. call(a) for a whereas visit_statement I presume would only be called let b = a;
|
Reminder, once the PR becomes ready for a review, use |
I would very much appreciate that: I'm very afraid of doing such a major rewrite of the entire of the feature. This isn't exactly a great reason, but from a personal point of view it is quite discouraging to have something working after months of work and then hear that nah, this'd actually better be done an entire different way :D in that sense I would personally prefer landing this and then starting on the rewrite while concurrently gathering feedback from users. |
We discussed this a little: The main question we had was: what was your thinking about usage of the EDIT:
Turns out we weren't even using the |
This comment has been minimized.
This comment has been minimized.
3b3699f to
c10c973
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
a739e9d to
b9d634a
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
View all comments
With this PR we now have basic functional Reborrow and CoerceShared traits. The current limitations are:
&mutwrappers is working (though I've not tested generic wrappers likeOption<&mut T>yet), but CoerceShared of&mutwrappers currently causes an ICE.The remaining tasks to complete before I'd consider this PR mergeable are:
Reborrow traits experiment: #145612
Co-authored by @dingxiangfei2009